feat: schema cache to prevent thundering herd - #1258
Conversation
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| let schema = shard.fetch_schema().await?; | ||
|
|
||
| *guard = schema.clone(); |
There was a problem hiding this comment.
in theory here could be a tricky data race with SchemaCache::global().clear();:
on replace_databases the following happens: clear -> new_databases.launch -> old_databases.shutdown
if for some reason we trigger replace_databases while schemas are still loading for old (like with retries due to errors) we may try to write into the cache that is basically associated with the new databases already.
The shutdown token would've be helpful, but this is called after the clear and launch, leaving this race window.
There is another race that is not important I think: lock -> fetch_schema -> reload_databases -> set to old guard. But it's not a big issue since the guard will not point to the cache map entry anymore and that'd be a separate mutex.
There was a problem hiding this comment.
I think I know what to do here. I'll add a global "config is reloading" signal that can cancel all of these long-running loops. I've been meaning to add something like this; making everything more deterministic.
There was a problem hiding this comment.
Might also have to be versioned too. Each config reload increments the config version so each Cluster knows which "epoch" it belongs to.
There was a problem hiding this comment.
*race condition, not a data race.
There was a problem hiding this comment.
Yeah, this one is kinda tough. We may need to refactor the config system from an ArcSwap to a watch, or something. to ensure this works across the whole app.
Right now, the race condition is real, but it may not have a real impact. i.e., if they race, they happen around the same time, so the schema is probably identical anyway. This was built specifically to busted when the user wants it to, e.g., they create a new table, then hit RELOAD in admin. Either way, the table already exists, and the schema loaded in the cache is fresh.
There was a problem hiding this comment.
I figured it out. We should attach all config-scoped "globals" to the Cluster. Duh. It's already versioned in a way that each config version gets its own Cluster, so as long as the cache is scoped to the lifetime of that Cluster, we don't have a race between config versions.
We should ban globals lol. Everything from now on should be Cluster-scoped.
Co-authored-by: Sage Griffin <sage@sagetheprogrammer.com>
| impl<'a> ClusterConfig<'a> { | ||
| /// Dependencies for creating a Cluster. | ||
| /// | ||
| /// TODO(lev): This is getting unruly. We may need a struct for a struct :) |
There was a problem hiding this comment.
maybe you'd like bon? https://bon-rs.com/guide/overview#method-builder
There was a problem hiding this comment.
Yeah. ClusterConfig was meant to be a "manual bon" for Cluster::new. So now we need a ClusterConfigConfig :)
Going to roll forward with this as-is. If there are any issues, I'll refactor.
3 participants
Sharded deployments with dozens / hundreds of users will stampede the servers to load the schema. With large schemas, e.g., thousands of tables, and multiple replicas, this is pretty slow and will cause issues.
The cache is using the database name and shard number, which uniquely identifies a host in
pgdog.toml.